Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lto: enabling lto at configure #21677

Closed
wants to merge 2 commits into from

Conversation

octaviansoldea
Copy link

@octaviansoldea octaviansoldea commented Jul 5, 2018

This modification allows for compiling with link time optimization (lto) using the flag --enable-lto.

Refs: #7400

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

This is a modification proposed towards enabling lto compilation. From some preliminary results, I observed that there are no major changes in performance when running Node-DC-EIS. However, when coupling lto with pgo, see also Refs: #21583, I observed improvements of even more than 10% in Node-DC-EIS.

The proposed solution, when enabling lto, manifests a test failure, which seems to be not very critical. In this context, please see comments and feedback from @addaleax at Refs: #7400.

The experiments were done on
Intel(R) Xeon(R) Platinum 8180 CPU @ 2.50GHz
and
Intel(R) Xeon(R) CPU E5-2699 v4 @ 2.20GHz.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jul 5, 2018
@Trott
Copy link
Member

Trott commented Jul 5, 2018

Note for anyone landing this: Subsystem should be build rather than lto.

@octaviansoldea If you want to amend the commit message for that, it will save someone a few keystrokes later on.

This modification allows for compiling with link time optimization (lto)
using the flag --enable-lto.

Refs: nodejs#7400
@octaviansoldea
Copy link
Author

Hello

Thank you for the comment. I will update the commit asap.

@octaviansoldea

@octaviansoldea
Copy link
Author

Hello

Thank you again for the comment. I have updated the label with amending. I am more than happy to receive feedback and comments.

@octaviansoldea

configure Outdated
for compiler in [(CC, 'c'), (CXX, 'c++')]:
ok, is_clang, clang_version, compiler_version = \
try_check_compiler(compiler[0], compiler[1])
if is_clang or compiler_version < version_checked:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

String-based version range checking like this is not foolproof, for example: '10.0.0' < '5.4.1' will evaluate to True. At the very least we would need a version comparison function that simply splits on '.' and compares each integer value between two version strings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or do a tuple comparison, e.g.,

node/configure

Line 727 in 0a78f7d

elif clang_version < (3, 4, 2) if is_clang else gcc_version < (4, 9, 4):

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello

Thank you for the comment. I will implement this asap.

@octaviansoldea

This commit is a refactoring for testing that the compilers used
for lto are gcc and g++ only.
@octaviansoldea
Copy link
Author

octaviansoldea commented Jul 7, 2018

Dear Reviewers,

Thank you for your valuable comments and feedback. I have implemented the suggested modifications. However, I would like to point out an issue I believe it is a problem to be addressed.

The calls

ok, is_clang, clang_version, gcc_version = try_check_compiler(CXX, 'c++')

and

ok, is_clang, clang_version, gcc_version = try_check_compiler(CC, 'c')

return clang_version and gcc_version as tuple of strings. This means, most probably, the subsequent tests

...
clang_version < (3, 4, 2) if is_clang else gcc_version < (4, 9, 4):
...
elif not is_clang and gcc_version < (4, 2, 0):
...

are a bit strange. Could you please tell if you want that I open a separate issue on this?

Thank you for valuable comments and feedback again.

@octaviansoldea

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 10, 2018
@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/15788/

Whoever lands this: Please be reminded to change the subsystem label to build: :)

@octaviansoldea
Copy link
Author

Dear Reviewers

I saw that "node-test-commit — tests failed". Could you please tell if is there anything I am supposed to do?

Thank you in advance,
@octaviansoldea

@jasnell
Copy link
Member

jasnell commented Jul 12, 2018

@octaviansoldea ... looks like an unrelated flaky failure. I'm running that one OSX test again: https://ci.nodejs.org/job/node-test-commit-osx/19766/

If that comes back green we should be able to land.

@addaleax
Copy link
Member

Landed in 32cad73, thanks for the PR! 🎉

@addaleax addaleax closed this Jul 13, 2018
addaleax pushed a commit that referenced this pull request Jul 13, 2018
This modification allows for compiling with link time optimization (lto)
using the flag --enable-lto.

Refs: #7400

PR-URL: #21677
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Jul 14, 2018
This modification allows for compiling with link time optimization (lto)
using the flag --enable-lto.

Refs: #7400

PR-URL: #21677
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
@misterdjules
Copy link

@octaviansoldea Is it correct that this PR does not enable Link Time Optimization by default? If so, is that the goal in the future and is there already a PR/issue where we can track that change/discussion?

@ChALkeR ChALkeR mentioned this pull request Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants